[DO NOT MERGE] Smart Escrows #818
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
Adds Smart Escrows support to xrpl-py by introducing new fields in escrow transactions, updating serialization definitions, extending fee logic, and reorganizing tests.
- Introduces
finish_functionanddatainEscrowCreate, andcomputation_allowanceinEscrowFinish - Updates binary codec definitions for new fields and WASM-related error codes
- Extends async fee calculation to include gas pricing and finish function costs
- Reorganizes escrow integration tests into a consolidated file and updates wallet faucet logic
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| xrpl/models/transactions/escrow_create.py | Add finish_function & data fields and validation logic |
| xrpl/models/transactions/escrow_finish.py | Add computation_allowance field |
| xrpl/core/binarycodec/definitions/definitions.json | Register new serialization definitions & WASM errors |
| xrpl/asyncio/wallet/wallet_generation.py | Update faucet URLs, map new network, add request timeout |
| xrpl/asyncio/transaction/main.py | Incorporate gas price and finish_function into fee calc |
| tests/unit/models/transactions/test_escrow_create.py | Add unit tests for new finish_function logic |
| tests/unit/asyn/wallet/test_wallet.py | Update unit tests for new faucet mappings |
| tests/integration/transactions/test_escrow.py | Consolidate escrow integration tests |
| tests/integration/it_utils.py | Add wait parameter to ledger acceptance helpers |
| pyproject.toml | Bump version to beta |
Comments suppressed due to low confidence (2)
tests/unit/models/transactions/test_escrow_create.py:8
- Add unit tests for the new
datafield inEscrowCreate, covering both serialization and validation scenarios whendatais provided or omitted.
def test_all_fields_valid(self):
xrpl/asyncio/transaction/main.py:610
ServerStateis not imported in this module, causing aNameError. Addfrom xrpl.models.requests import ServerState(or the correct import) at the top.
server_state = await client._request_impl(ServerState())
| @@ -67,6 +67,10 @@ class EscrowCreate(Transaction): | |||
| fulfilled. | |||
| """ | |||
|
|
|||
| finish_function: Optional[str] = None | |||
|
|
|||
| data: Optional[str] = None | |||
There was a problem hiding this comment.
The new data field lacks a corresponding entry in definitions.json, so it won't be serialized. Add a "Data" definition under the escrow fields section with the appropriate isSerialized, nth, and type settings.
Copilot uses AI. Check for mistakes.
| @@ -220,7 +228,7 @@ async def _request_funding( | |||
| json_body = {"destination": address, "userAgent": user_agent} | |||
| if usage_context is not None: | |||
| json_body["usageContext"] = usage_context | |||
| response = await http_client.post(url=url, json=json_body) | |||
| response = await http_client.post(url=url, json=json_body, timeout=10) | |||
There was a problem hiding this comment.
Hard-coding timeout=10 duplicates the existing _TIMEOUT_SECONDS constant. Use timeout=_TIMEOUT_SECONDS to keep timeout behavior consistent.
| response = await http_client.post(url=url, json=json_body, timeout=10) | |
| response = await http_client.post(url=url, json=json_body, timeout=_TIMEOUT_SECONDS) |
Copilot uses AI. Check for mistakes.
| if transaction.transaction_type == TransactionType.ESCROW_CREATE: | ||
| escrow_create = cast(EscrowCreate, transaction) | ||
| if escrow_create.finish_function is not None: | ||
| base_fee += 1000 |
There was a problem hiding this comment.
[nitpick] The 1000 finish-function surcharge is a magic number. Extract it into a named constant (e.g., FINISH_FUNCTION_FEE) and document its origin.
| base_fee += 1000 | |
| base_fee += FINISH_FUNCTION_FEE |
Copilot uses AI. Check for mistakes.
| @@ -63,6 +63,8 @@ class EscrowFinish(Transaction): | |||
| """Credentials associated with sender of this transaction. The credentials included | |||
| must not be expired.""" | |||
|
|
|||
| computation_allowance: Optional[int] = None | |||
There was a problem hiding this comment.
[nitpick] Add validation in _get_errors to ensure that computation_allowance is only set when a finish_function is provided, otherwise reject or warn.
Copilot uses AI. Check for mistakes.
High Level Overview of Change
Adds xrpl-py support for Smart Escrows.
Also reorganizes the Escrow integration tests into one file.
Context of Change
XRPLF/XRPL-Standards#270
Type of Change
Did you update CHANGELOG.md?
Test Plan
Added some basic tests
Integration tests expected to fail right now, since this is a new feature.